Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow ProfileTypeBuilder to pass its three filters list to both implementations of ProfileType #4325

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

obones
Copy link

@obones obones commented Jul 22, 2024

This pull request moves getSupportedChannelTypeUIDs and getSupportedItemTypesOfChannel to the ProfileType interface so that StateProfileType and TriggerProfileType can both take the three filter lists given by ProfileTypeBuilder

It fixes #4293 and will allow bundle writers to limit their transformation profiles to just the channels/items that are related to their profiles.

@obones obones requested a review from a team as a code owner July 22, 2024 08:54
…o the ProfileType interface so that StateProfileType and TriggerProfileType can both take the three filter lists given by ProfileTypeBuilder

Fixes openhab#4293

Signed-off-by: Olivier Sannier <[email protected]> (github: obones)
Signed-off-by: OBones <[email protected]>
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pleas also check why there are test failures in ProfileTypeResourceTest.

@obones
Copy link
Author

obones commented Jul 25, 2024

I don't know if it's the same for everyone, but it takes a really long time to build the entire openhab-core project, and I can't even get it to work properly in its entirety with VSCode under Windows.
Hence the reason why I pushed commits to see what GitHub actions would say and try to figure out which sub project is complaining.
I'll make sure to squash those commits into a more readable one.

@J-N-K
Copy link
Member

J-N-K commented Jul 25, 2024

No need for squashing, I can do that during merge. In fact, if you squash and force-push usually the comment history is messed up, so better just push additional commits.

@obones
Copy link
Author

obones commented Jul 26, 2024

I have just pushed some changes that I believe are addressing your fine remarks.
Do not hesitate to tell me if I missed other things.

Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for you work and sorry for being a PITA here.

Comment on lines 237 to 242
if (!(profileType.getSupportedItemTypes().isEmpty()
|| profileType.getSupportedItemTypes().contains(itemType))
|| !(((TriggerProfileType) profileType).getSupportedChannelTypeUIDs().isEmpty()
|| ((TriggerProfileType) profileType).getSupportedChannelTypeUIDs()
.contains(channel.getChannelTypeUID()))) {
|| !(profileType.getSupportedChannelTypeUIDs().isEmpty()
|| profileType.getSupportedChannelTypeUIDs().contains(channel.getChannelTypeUID()))
|| !(profileType.getSupportedItemTypesOfChannel().isEmpty()
|| profileType.getSupportedItemTypesOfChannel().contains(itemType))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks really horrible. It would probably be better to

   Collection<String> supportedItemTypes = profileType.getSupportedItemTypes();
   ...
   if ((!supportedItemType.isEmpty() && !supportedItemTypes.contains(itemType)) || ...) {

Why do you check profileType.getSupportedItemTypesOfChannel? A profile can change the item-type:

  • The timestamp update profile works with all channel-types (e.g. a humidity channel with supported item-type Number:Dimensionless) but can only be linked to DateTime items. So a check would fail (in fact, currently the profile does not provide information about accepted channel types, so this will not cause an issue, but in principle it could).
  • A profile could map numeric values to strings, so the channel would support Number, but the profile String. In this case you could never link them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it looks horrible, but it was jut to "fit in" with the already existing code.

As to why I do the check, well it's because I did a global search for the interface methods and made sure both methods are called in all the places where one was called following an instanceof call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, the style was not good before either and if you want to keep it: ok. But the getSupportedItemTypesOfChannel is excluding valid combinations.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm definitely in favor of changing the code style here, but as always when I'm submitting a pull request, I try to limit my changes to the code logic, adapting to the local style I encounter.
I'll try to come up with a more sensible way of writing this up.

return true;
}
private boolean profileTypeMatchesChannelType(ProfileType profileType, ChannelType channelType) {
@Nullable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need @Nullable annotation for local variables.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My IDE (VSCode) disagrees and outputs a warning saying that the value cannot be null. That's what I've been doing in my plugin bundles, but I can remove it if you prefer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's wrong. The default locations for @NonNullByDefault are PARAMETER, RETURN_TYPE, FIELD, TYPE_BOUND, TYPE_ARGUMENT, so it does not apply to local variables.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, I have removed them

…the two other locations where ProfileType are tested for applicability

Signed-off-by: OBones <[email protected]>
@obones
Copy link
Author

obones commented Jul 29, 2024

The more I look at how things are going, the more I feel it would be beneficial to have a ProfileTypeUtil abstract class that provides two methods receiving (among other things) a Channel for the first and a ChannelType for the second and those methods would return a boolean indicating if a given ProfileType applies to the channnel(type)
This would avoid very similar code to be placed in the three different places it is currently placed.
Does that make any sense to you?

@obones
Copy link
Author

obones commented Sep 28, 2024

@J-N-K , sorry to ping you directly, but do you have an opinion on my last suggestion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ProfileTypeBuilder.withSupportedChannelTypeUIDs is ignored by StateProfileTypeImpl
2 participants